-
Notifications
You must be signed in to change notification settings - Fork 1
Feature/db error handling category #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds integrity-aware error handling to category create/update flows (catching UNIQUE violations for Category.name and category-subcategory links), caches the single-column UniqueConstraint for Category.name, switches product retrieval to relationship-based filtering, and adds a test asserting UNIQUE constraint behavior when linking subcategories. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as CategoryCollection
participant DB as SQLAlchemy Session
Client->>API: POST/PUT Category (name, subcategories)
API->>DB: session.add(instance) / commit()
alt Commit succeeds
DB-->>API: commit OK
API-->>Client: 200/201
else IntegrityError with UniqueViolation on name
DB-->>API: IntegrityError(UniqueViolation)
API->>DB: session.rollback()
API-->>Client: 409 Conflict (duplicate category name)
else IntegrityError with UniqueViolation on category_subcategory pk
DB-->>API: IntegrityError(UniqueViolation)
API->>DB: session.rollback()
API-->>Client: 409 Conflict (duplicate category-subcategory link)
end
sequenceDiagram
autonumber
actor Client
participant API as CategoryProducts.get
participant ORM as SQLAlchemy ORM
Client->>API: GET /categories/:id/products
API->>ORM: Product.query.filter(Product.subcategories.any(Subcategory.categories.any(id=:id)))
ORM-->>API: product list
API-->>Client: 200 with products
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/migrated_routes/category.py(5 hunks)tests/test_relationships.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_relationships.py (2)
tests/conftest.py (4)
create_authenticated_headers(43-50)create_category(54-63)create_subcategory(67-78)client(14-21)app/migrated_routes/category.py (1)
put(153-215)
app/migrated_routes/category.py (1)
app/models.py (3)
Category(77-93)Product(116-134)Subcategory(96-113)
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
1 similar comment
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_relationships.py (2)
1-2: Backend-coupled import ties the test to SQLiteImporting
sqlite3for assertions below makes the test engine-specific. Consider asserting engine-agnostic invariants (HTTP 409 when API maps integrity errors) or parametrize by backend.If you plan to run tests on Postgres too, add a variant that expects a 409 response instead of a raw DB exception.
102-113: Test asserts driver-specific error; consider a backend-agnostic variantThis expects
sqlite3.IntegrityErrorand “UNIQUE constraint failed”. On Postgres, the route maps duplicate links to HTTP 409 (after fixes). Add a complementary test that exercises the API behavior (409) and keep this as SQLite-only if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/migrated_routes/category.py(5 hunks)tests/test_relationships.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
app/migrated_routes/category.py
116-116: Use raise without specifying exception name
Remove exception name
(TRY201)
213-213: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (3)
app/migrated_routes/category.py (3)
105-116: Postgres 409 path unreachable; compare against resolved DB constraint names and re‑raise correctlyComparing to
CategoryCollection._NAME_UNIQUE_CONSTRAINT.namewon’t match Postgres’ actual constraint (e.g.,category_name_key). Also prefer bareraiseto preserve traceback. This mirrors an earlier review note.try: db.session.add(category) db.session.commit() except IntegrityError as ie: db.session.rollback() - if ( - isinstance(ie.orig, UniqueViolation) - and ie.orig.diag.constraint_name - == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name - ): + if ( + isinstance(getattr(ie, "orig", None), UniqueViolation) + and getattr(getattr(ie.orig, "diag", None), "constraint_name", None) + in CategoryCollection._resolve_name_unique_constraint_names() + ): abort(409, message="Category with this name already exists") - raise ie + raise
203-214: Duplicate link 409 path also unreachable on Postgres; resolve PK name via inspector and use bare re‑raise
category_subcategory.primary_key.nameis typicallyNoneunless explicitly named; Postgres reports something likecategory_subcategory_pkey. Resolve it from the DB and compare againstdiag.constraint_name. This mirrors an earlier review note.try: db.session.commit() except IntegrityError as ie: db.session.rollback() - if ( - isinstance(ie.orig, UniqueViolation) - and ie.orig.diag.constraint_name - == category_subcategory.primary_key.name - ): - abort(409, message="Category and subcategory already linked") - raise ie + if isinstance(getattr(ie, "orig", None), UniqueViolation): + pk_name = category_subcategory.primary_key.name + if not pk_name: + pk_info = inspect(db.engine).get_pk_constraint(category_subcategory.name) + pk_name = pk_info.get("name") + if getattr(getattr(ie.orig, "diag", None), "constraint_name", None) == pk_name: + abort(409, message="Category and subcategory already linked") + raise
315-317: Approve code changes: relationship-based filtering reads clearly; nestedany()is idiomatic, and thecategory_subcategorytable already defines a composite primary key on(category_id, subcategory_id), supporting FKs, plus an index.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/migrated_routes/category.py(5 hunks)tests/test_relationships.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
app/migrated_routes/category.py
116-116: Use raise without specifying exception name
Remove exception name
(TRY201)
213-213: Use raise without specifying exception name
Remove exception name
(TRY201)
🔇 Additional comments (3)
app/migrated_routes/category.py (3)
31-43: Import-time TypeError: staticmethod invoked inside the class bodyAt Line 42 you call
_get_name_unique_constraint()while the class body is still executing. Because Line 31 decorates it with@staticmethod, the name in the class namespace is astaticmethoddescriptor, soCategoryCollectionfails to import withTypeError: 'staticmethod' object is not callable. Beyond that, caching the ORMUniqueConstraintinstance won’t help you match the runtime constraint name on Postgres anyway (it staysNonewhen the column was declared withunique=True). Please replace this with a lazily evaluated inspector-based helper that returns the real constraint names.-from sqlalchemy import UniqueConstraint, exists +from functools import lru_cache +from sqlalchemy import UniqueConstraint, exists, inspect ... - @staticmethod - def _get_name_unique_constraint(): - name_col = Category.__table__.c.name - return next( - con - for con in Category.__table__.constraints - if isinstance(con, UniqueConstraint) - and len(con.columns) == 1 - and con.columns.contains_column(name_col) - ) - - _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint() + @staticmethod + @lru_cache(maxsize=1) + def _name_unique_constraint_names(): + insp = inspect(db.engine) + return tuple( + uc["name"] + for uc in insp.get_unique_constraints(Category.__tablename__) + if uc.get("name") and set(uc.get("column_names") or []) == {"name"} + )
105-116: Duplicate-name 409 branch is still unreachable on PostgresLine 113 compares
ie.orig.diag.constraint_nameagainst_NAME_UNIQUE_CONSTRAINT.name, but—as noted previously—that attribute isNonefor constraints produced byColumn(unique=True). In Postgres the driver reportscategory_name_key, so this equality never succeeds and we still leak the IntegrityError as a 500. After adopting the inspector helper above, rework this block to compare against the resolved constraint names (usegetattrto guard against engines that lack.diag). Also re-raise with bareraiseto preserve the original traceback.- if ( - isinstance(ie.orig, UniqueViolation) - and ie.orig.diag.constraint_name - == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name - ): + constraint_name = getattr(getattr(ie.orig, "diag", None), "constraint_name", None) + if ( + isinstance(ie.orig, UniqueViolation) + and constraint_name in CategoryCollection._name_unique_constraint_names() + ): abort(409, message="Category with this name already exists") - raise ie + raise
203-213: Association duplicate guard never fires because the PK name is None
category_subcategory.primary_key.name(Line 210) returnsNoneunless the association table’s primary key was explicitly named—Postgres emitscategory_subcategory_pkey, so this branch is dead and the update endpoint still returns 500. Resolve the actual PK name viainspect(db.engine).get_pk_constraint(category_subcategory.name)["name"](cache it) and compare against that result. Guard access to.diagjust like in the name-constraint case, and use bareraiseafterward.- if ( - isinstance(ie.orig, UniqueViolation) - and ie.orig.diag.constraint_name - == category_subcategory.primary_key.name - ): + pk_name = category_subcategory.primary_key.name + if not pk_name: + pk_name = inspect(db.engine).get_pk_constraint(category_subcategory.name)["name"] + constraint_name = getattr(getattr(ie.orig, "diag", None), "constraint_name", None) + if isinstance(ie.orig, UniqueViolation) and constraint_name == pk_name: abort(409, message="Category and subcategory already linked") - raise ie + raise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/migrated_routes/category.py (2)
105-116: Make duplicate-name detection portable across drivers (fallback to sqlstate 23505).If tests/dev use SQLite,
ie.origwon’t beUniqueViolation; adding a sqlstate fallback keeps the 409 path consistent without coupling to psycopg2 type checks.Apply:
- if ( - isinstance(ie.orig, UniqueViolation) - and ie.orig.diag.constraint_name - == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name - ): + if ( + (isinstance(ie.orig, UniqueViolation) + or getattr(getattr(ie.orig, "diag", None), "sqlstate", None) == "23505") + and getattr(getattr(ie.orig, "diag", None), "constraint_name", None) + == CategoryCollection._NAME_UNIQUE_CONSTRAINT.name + ): abort(409, message="Category with this name already exists")Based on learnings.
4-6: Driver-specific import is fine; consider a lightweight portability guard.Using
psycopg2.errors.UniqueViolationties behavior to Postgres. With the sqlstate fallback in handlers, portability improves without removing this import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/migrated_routes/category.py(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-27T20:27:42.172Z
Learnt from: piyush-jaiswal
PR: piyush-jaiswal/ecommerce-rest-api#18
File: app/migrated_routes/category.py:203-213
Timestamp: 2025-09-27T20:27:42.172Z
Learning: In this ecommerce REST API codebase, SQLAlchemy naming conventions are configured in app/__init__.py with PostgreSQL-compatible patterns, ensuring that association table primary keys like category_subcategory.primary_key.name return actual constraint names (e.g., "category_subcategory_pkey") rather than None, making IntegrityError constraint name comparisons work correctly.
Applied to files:
app/migrated_routes/category.py
🧬 Code graph analysis (1)
app/migrated_routes/category.py (1)
app/models.py (3)
Category(77-93)Product(116-134)Subcategory(96-113)
🔇 Additional comments (2)
app/migrated_routes/category.py (2)
315-317: LGTM: Relationship-based filtering is clearer than manual joins.This reads well and should generate efficient EXISTS subqueries with proper FK/PK indexes.
31-42: Blocker: @staticmethod call inside class body is not callable (TypeError at import time).Calling a
@staticmethodwithin the class body invokes thestaticmethoddescriptor, which isn’t callable and raises at import time. Call the underlying function via.__func__(or move the assignment after the class definition).Apply:
- _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint() + _NAME_UNIQUE_CONSTRAINT = _get_name_unique_constraint.__func__()Likely an incorrect or invalid review comment.
|
@coderabbitai resolve |
✅ Actions performedComments resolved. |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests